-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change plugin config to be specified as a list #383
Conversation
This will NOT be a breaking change. In essence, it will make the config look like this (see bottom for plugins change):
|
6f7001b
to
7a2002a
Compare
@gotyaoi This is a change to the way that plugins are configured, I thought you would want to be in the loop on it. Makes plugins configured in a way similar to outputs, which means that users can specify multiple plugin types with completely different configurations. |
cc @pauldix @beckettsean @gunnaraasen this PR would change the way the config file looks but is NOT a breaking change |
The only things I can think of is that for some plugins, like the CPU plugin, it may not make sense to have multiple copies, and that other plugins, like procstat, have already worked around this somewhat by having the plugin be a container, which controls a list of the actual things to check. Granted if there are multiple plugins doing this, and I think there are, it may make sense to have a generic solution, but procstat and the others might need to be changed to take advantage of it. |
@gotyaoi Agreed, I was hoping to get away from plugins like procstat and httpjson, which have to do a lot of internal threading and error handling. I think it would be better for the telegraf agent to handle the threading. Even the CPU plugin, while simple, could have a use-case for multiple instances. For example, what if you wanted cpu_time* for cpu-total, but not for cpu0, cpu1, etc. Currently that's not possible, but with this change you could do that like this:
This particular scenario is not very likely, but I think it would be better to have this flexibility. |
Sure, if it's not a breaking change +1 |
Works for me |
5e07419
to
4125523
Compare
@gotyaoi This is finally completed 😅 I made some significant changes, and namely I realized that almost all of the configuration merging had to be negated in order to accomodate specifying multiple plugins as a list. Since they are a list, there is not really a way to get it to understand when it should merge and overwrite a plugin or not. I'm sorry that I have removed some of the power of the configuration changes that you made, but I would like to move Telegraf towards a configuration that allows for plugins to be lists, and for plugins to not need to deal with large amounts of parallelism internally. |
4125523
to
1f9d65e
Compare
No worries. My code made sense in a context where there is only one instance of a given struct type, and that is no longer the case. There are potentially a few issues I can see though. On mobile right now, but I'll add some comments when I get home. |
} | ||
|
||
// NewAgent returns an Agent struct based off the given Config | ||
func NewAgent(config *Config) (*Agent, error) { | ||
func NewAgent(config *config.Config) (*Agent, error) { | ||
agent := &Agent{ | ||
Tags: make(map[string]string), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Tags have been shuffled around a bit over time. There currently seems to be Tags attributes on both the Agent and the Config. I think the one on the Config is the correct one, so removing the one from the Agent may reveal a few places where it's still being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, makes sense, I'll make that change
Last thing wouldn't relate to a specific line: My changes sort of tried to move all the details from the Config to the Agent, once all the config files were parsed. This seems to go back to keeping around the Config with the details in it, which is fine, but then there's the matter of the Agent's config, which is copied into the Agent from the Config. So now the Config has an *Ast.Table hanging around in it's agent attribute, and the agent refers to itself for some values, and to the config for other values, which feels sort of strange. Perhaps the various attributes on the agent struct should actually live on the Config, and the agent just holds a Config, which it refers to for details on what to do. This even enables a potentially nice path to config reloading in future, simply by building the new config in the background, then subbing out the Agent's Config attribute in one go. |
@gotyaoi I'll make changes to have the agent config be held within config, I think that makes more sense |
continue | ||
} | ||
name := entry.Name() | ||
if name[len(name)-5:] != ".conf" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because #395 made me stare at it for a few minutes, I noticed a possible panic in my directory loading code, if the filename is too small. Since you're rewriting here anyway, you might want to include a change here:
if len(name) < 6 || name[len(name)-5:] != ".conf" {
The length check should protect the string slice from going out of bounds via short circuit evaluation of the condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix that, good catch
724bb7a
to
58946b5
Compare
This makes plugin configuration more similar to output configuration, where we can specify multiple plugins as a list. The idea behind this is that the Telegraf agent can handle the multi-processing and error handling better than each plugin handling that internally. This will also allow for having different plugin configurations for different instances of the same type of plugin.
58946b5
to
ae62ce8
Compare
ae62ce8
to
a5f2d5f
Compare
This makes plugin configuration more similar to output configuration,
where we can specify multiple plugins as a list. The idea behind this is
that the Telegraf agent can handle the multi-processing and error
handling better than each plugin handling that internally. This will
also allow for having different plugin configurations for different
instances of the same type of plugin.